-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ZEPPELIN-1850] Introduce Webpack (front) #1805
Conversation
So coool! I tested following your instructions and it worked like a charm 👍 LGTM!!! |
"dev:server": "webpack-dev-server --hot", | ||
"dev:watch": "grunt watch-webpack-dev", | ||
"prestart": "grunt pre-webpack-dev", | ||
"start": "npm-run-all --parallel dev:server dev:watch", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably it's a nit; since there is default command npm start
to run the script in npm, I think it will be better to use another for this. How about dev
instead of start
? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for review :) @AhyoungRyu
Since there is default command
npm start
....
How aboutdev
instead ofstart
? :)
So, you mean start:server
and start:watch
. right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just meant we run dev server script \w npm run start
for now, so I suggested npm run dev
for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see! Fixed in 61dd5e5
702aaac
to
b5bfe9d
Compare
@1ambda To quickly test this branch, I tried to change some css components and html, js files. It's working really nicely(and super fast). Great work indeed! Seems it solved ZEPPELIN-1805 as well :) |
@AhyoungRyu I didn't know about ZEPPELIN-1805 but good to hear that it is solved by this PR. Also, docs was updated to include |
8442eba
to
e467b96
Compare
@1ambda Great improvement! One thing is, previously console.log() was printed only in dev mode. After this change, console.log() is printed both dev and dist. If it is not very difficult, can console.log() be muted in non-dev mode? Other than that, LGTM. |
e467b96
to
6aa25b8
Compare
@Leemoonsoo Thanks! Remove
|
5e1696f
to
920589b
Compare
Tested LGTM and merge to master if there're no further discussions. |
### What is this PR for? Removed [strip-loader](https://issues.apache.org/jira/browse/ZEPPELIN-1850) which was added to remove `console.log` in production. But we can achieve the same result by using uglify option. I missed it in #1805 - `strip-loader` has a bug as you can see in below (screenshot section) - Deleting it help us to keep the package list simple - Also using the same configuration for uglify both in webpack and grunt will ensure that vendor package have the consistent behavior when they are managed by webpack Also, this should be merged before #1812 since we need to modify `yarn.lock` in #1812 ### What type of PR is it? [Bug Fix | Improvement] ### Todos Done at once ### What is the Jira issue? [ZEPPELIN-1850](https://issues.apache.org/jira/browse/ZEPPELIN-1850) ### How should this be tested? 1. Add this code to any js file in master branch and execute `npm run build` to lint. You will get an error ``` console.log('editor isn\'t loaded yet, returning..'); ``` 2. Test the same command and code within this branch. It will work. ### Screenshots (if appropriate) ``` ERROR in ./src/app/notebook/paragraph/paragraph.controller.js Module build failed: SyntaxError: Unexpected token, expected , (197:8) 195 | } 196 | } > 197 | }); | ^ 198 | } else { 199 | BootstrapDialog.confirm({ 200 | closable: true, ./src/index.js 53:0-59 Child html-webpack-plugin for "index.html": + 1 hidden modules Running "useminPrepare:html" (useminPrepare) task Configuration changed for concat, uglify, cssmin Running "concurrent:dist" (concurrent) task Running "svgmin:dist" (svgmin) task Total saved: 0 B Done, without errors. Execution Time (2016-12-30 06:21:32 UTC) loading tasks 121ms ▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇ 70% svgmin:dist 51ms ▇▇▇▇▇▇▇▇▇▇▇▇▇▇ 30% Total 172ms Running "copy:styles" (copy) task Copied 17 files Done, without errors. Execution Time (2016-12-30 06:21:32 UTC) loading tasks 127ms ▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇ 77% copy:styles 38ms ▇▇▇▇▇▇▇▇▇▇▇ 23% Total 165ms Running "postcss:dist" (postcss) task >> 17 processed stylesheets created. >> No "concat" targets found. Warning: Task "concat" failed. Use --force to continue. Aborted due to warnings. Execution Time (2016-12-30 06:21:30 UTC) loading tasks 117ms ▇▇▇▇ 6% useminPrepare:html 22ms ▇ 1% concurrent:dist 1.5s ▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇ 80% postcss:dist 252ms ▇▇▇▇▇▇▇▇▇ 13% Total 1.9s npm ERR! Darwin 15.6.0 npm ERR! argv "/Users/lambda/.nvm/versions/node/v6.9.1/bin/node" "/Users/lambda/.nvm/versions/node/v6.9.1/bin/npm" "run" "build" ``` ### Questions: * Does the licenses files need update? - NO * Is there breaking changes for older versions? - NO * Does this needs documentation? - NO Author: 1ambda <1amb4a@gmail.com> Closes #1823 from 1ambda/ZEPPELIN-1850/remove-strip-loader-in-webpack and squashes the following commits: 137bcdd [1ambda] review: Fix invalid uglify plugin opt 3d3a581 [1ambda] fix: Remove strip-loader to avoid single quote error
What is this PR for?
1. The purpose of this PR is introducing webpack to zeppelin-web
import
,export
features.2. This PR is designed to improve build process gradually!!!
These will be handled by additional PRs.
3. This PR is not big
use strict
, importingzeppelin.js
. (Also includes fixups for [MINOR] fix: Validate before setting focus to paragraphs #1802 and [MINOR] fix: Initialize zeppelin var in visualization/builtin/*.js #1803)Gruntfile.js
,package.json
,webpack.config.js
What type of PR is it?
[Improvement]
Todos
zeppelin
problemnpm run test
)What is the Jira issue?
ZEPPELIN-1850
How should this be tested?
cd zeppelin-web && rm -rf node_modules bower_components node
npm install
npm run test
npm run build
npm run dev
and openlocalhost:9000
: check live-reload works regarding to html, css, js filescd .. && mvn clean package -pl 'zeppelin-web' -DskipTests && ./bin/zeppelin-daemon.sh restart
and openlocalhost:8080
Screenshots (if appropriate)
N/A
Questions: